Skip to content

http: don't emit 'readable' after 'close'#32277

Closed
ronag wants to merge 3 commits intonodejs:masterfrom
nxtedition:http-close-readable
Closed

http: don't emit 'readable' after 'close'#32277
ronag wants to merge 3 commits intonodejs:masterfrom
nxtedition:http-close-readable

Conversation

@ronag
Copy link
Member

@ronag ronag commented Mar 15, 2020 •
edited
Loading

An edge case could emit 'readable' after 'close' and 'close' before 'end'.

Refs: #28710

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

@ronag ronag added the http Issues or PRs related to the http subsystem. label Mar 15, 2020
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Mar 15, 2020

CI: https://ci.nodejs.org/job/node-test-pull-request/29840/

@ronag ronag requested a review from addaleax March 15, 2020 12:32
@ronag
Copy link
Member Author

ronag commented Mar 15, 2020

I guess optimally clearIncoming would do its work on the socket and not the request object. But that's a bit more complicated.

@ronag ronag force-pushed the http-close-readable branch from 600401d to c48885a Compare March 15, 2020 12:39
@ronag ronag requested a review from jasnell March 19, 2020 00:20
@ronag
Copy link
Member Author

ronag commented Mar 19, 2020

@nodejs/http

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Mar 19, 2020

CI: https://ci.nodejs.org/job/node-test-pull-request/29917/

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Mar 19, 2020

CI: https://ci.nodejs.org/job/node-test-pull-request/29924/

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Mar 19, 2020

CI: https://ci.nodejs.org/job/node-test-pull-request/29930/

jasnell
jasnell reviewed Mar 19, 2020
View reviewed changes
jasnell
jasnell reviewed Mar 19, 2020
View reviewed changes
@ronag ronag requested a review from jasnell March 26, 2020 15:51
@ronag
Copy link
Member Author

ronag commented Mar 26, 2020

@nodejs/http

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Mar 26, 2020

CI: https://ci.nodejs.org/job/node-test-pull-request/30096/

@ronag
Copy link
Member Author

ronag commented Mar 27, 2020

Suspicious failure on CI.

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Mar 27, 2020

CI: https://ci.nodejs.org/job/node-test-pull-request/30117/

@addaleax addaleax added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Mar 29, 2020
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Mar 29, 2020

CI: https://ci.nodejs.org/job/node-test-pull-request/30202/

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Mar 29, 2020

CI: https://ci.nodejs.org/job/node-test-pull-request/30211/

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Mar 30, 2020

CI: https://ci.nodejs.org/job/node-test-pull-request/30226/

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Mar 30, 2020

CI: https://ci.nodejs.org/job/node-test-pull-request/30240/

@addaleax
Copy link
Member

addaleax commented Mar 30, 2020

Landed in e034f5c

@addaleax addaleax closed this Mar 30, 2020
addaleax pushed a commit that referenced this pull request Mar 30, 2020
@ronag @addaleax
http: don't emit 'readable' after 'close'
e034f5c 
PR-URL: #32277
Refs: #28710
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
addaleax pushed a commit that referenced this pull request Mar 30, 2020
@ronag @addaleax
http: don't emit 'readable' after 'close'
7c3c062 
PR-URL: #32277
Refs: #28710
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
@targos targos mentioned this pull request Apr 13, 2020
Merged
targos pushed a commit that referenced this pull request Apr 22, 2020
@ronag @targos
http: don't emit 'readable' after 'close'
3b02042 
PR-URL: #32277
Refs: #28710
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
@targos targos mentioned this pull request Apr 22, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jasnell jasnell jasnell approved these changes

@addaleax addaleax addaleax approved these changes

Assignees

No one assigned

Labels

author ready PRs that have at least one approval, no pending requests for changes, and a CI started. http Issues or PRs related to the http subsystem.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@ronag @nodejs-github-bot @addaleax @jasnell